Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: make dedupe faster at I/O level #3237

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: make dedupe faster at I/O level #3237

wants to merge 1 commit into from
+69 −36

Conversation

alpha-ulrich
Copy link

Summary:
Uses Arc to ensure that dedupe requests do not generate new clones of the response, but rather point to the same response using the Atomically referenced counter.

Issue Reference(s):
Fixes #3028

/claim #3028

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests. [Modified relevant tests after code changes]
  • I have updated the documentation accordingly. [No update required for documentation]
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@algora-pbc algora-pbc bot mentioned this pull request Jan 3, 2025
Open
@github-actions github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label Jan 3, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 3, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.32%. Comparing base (e3a6026) to head (d3850c3).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
src/core/data_loader/dedupe.rs 85.18% 4 Missing ⚠️
src/core/ir/eval.rs 75.00% 1 Missing ⚠️
src/core/ir/eval_io.rs 92.30% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3237      +/-   ##
==========================================
- Coverage   86.32%   86.32%   -0.01%     
==========================================
  Files         282      282              
  Lines       28798    28821      +23     
==========================================
+ Hits        24860    24879      +19     
- Misses       3938     3942       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tusharmath tusharmath added the ci: lint Automatically fix the linters issues and make a commit label Jan 4, 2025
@tusharmath
Copy link
Contributor

@alpha-ulrich Thanks!
Can you add some e2e benchmarks to see if there is an observed difference in performance? We eventually end up cloning the data in the PR, so I am curious to see if Arc is making a difference in performance.

@alpha-ulrich
Copy link
Author

@tusharmath sure, I'll see what can be done, later today.
I'll have a look at the graphql-benchmarks repository, to see if I can reuse some parts from there to accomplish this.

If there are any specific metrics that you would want reported, do let me know.

@github-actions GitHub Actions
Copy link

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels Jan 10, 2025
@github-actions GitHub Actions
Copy link

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
🙋 Bounty claim ci: lint Automatically fix the linters issues and make a commit state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. type: feature Brand new functionality, features, pages, workflows, endpoints, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: make dedupe faster at I/O level.
2 participants
@alpha-ulrich @tusharmath